-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Mirror what native build system does for enabling clang modules #9188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
daveinglis
commented
Sep 25, 2025
- the native builder only enables clang modules on Darwin, so we will need to do the same with swift-build.
@swift-ci test |
|
||
func configureProjectBuildSettings(_ buildSettings: inout ProjectModel.BuildSettings) { | ||
/* empty */ | ||
// This is parity to the native build, But we should investigate what it would take to get this working on platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking the current platform in BuildParameters, we should use platform conditionals here so that the PIF remains platform-agnostic. The subscript on buildSettings has a for
parameter you can use to add platform conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably try at some point to avoid giving the PIF builder access to build parameters to help prevent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The package model will likely need enhancement so that we can properly express all the conditionals users will need, especially around platforms, but that's in the works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that using the platform conditions is okay for now:
buildSettings.platformSpecificSettings[.linux]![.FOO] = "bar"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why are we doing this in the PIF delegate implementation for SwiftPM? To be clear, we should reserve the delegate for SwiftPM only patches.
If this is a general patch (eg, Xcode, etc) we should hardcode it in PackagePIFBuilder.addProjectBuildSettings
instead. For instance, Xcode has it's own independent PackagePIFBuilder.BuildDelegate
implementation and, as such, won't inherit such configuration as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, it doesn't look like you can set single value settings using buildSettings.platformSpecificSettings
Looks like swif-build ProjectModel.BuildSettings
would need to work to support this.
} | ||
|
||
settings[.USE_HEADERMAP] = "NO" | ||
settings[.OTHER_SWIFT_FLAGS].lazilyInitializeAndMutate(initialValue: ["$(inherited)"]) { $0.append("-DXcode") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should address this separately and conditionalize it on a new field in the PIFBuilderParameters
blocked by swiftlang/swift-build#827 |
9929ece
to
cf4866a
Compare
@swift-ci test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but because the new settings API handles $(inherited) for multivalued settings a little differently it may be worth comparing the PIF of some simple packages before and after to double-check there aren't any unexpected changes.
@swift-ci test windows |
cf4866a
to
20aa6ed
Compare
Ok, discovered the previous encoding filtered out multi-value settings with just [$(inherited)], which with this new change did cause a lot of these ending up in the PIF, so I update the helper to not initialize these and now the PIF look correct. (used swiftPM as the package to compare the PIF with before and after) |
@swift-ci test |
@swift-ci test windows |
20aa6ed
to
c04d067
Compare
@swift-ci test |
@swift-ci test windows |
- the native builder only enables clang modules on Darwin, so we will need to do the same with swift-build. - update settings handling to use new subscript with platform specifier instead of deprecated platformSpecificSettings property.
c04d067
to
5a1485e
Compare
@swift-ci test |